Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TbV log clustering by scene (i.e. spatial location). #26

Merged
merged 10 commits into from
Apr 12, 2022

Conversation

senselessdev1
Copy link
Contributor

PR Summary

Some users are interested in clustering TbV logs by spatial location, to identify before/after logs. This PR adds annotation of which logs are clustered together, in which cities. Each log in each cluster shares some visual overlap.

Testing

In order to ensure this PR works as intended, it is:

  • unit tested.
  • other or not applicable (additional detail/rationale required)

Compliance with Standards

As the author, I certify that this PR conforms to the following standards:

  • Code changes conform to PEP8 and docstrings conform to the Google Python style guide.
  • A well-written summary explains what was done and why it was done.
  • The PR is adequately tested and the testing details and links to external results are included.

@senselessdev1 senselessdev1 changed the title Add TbV log cluster by scene (i.e. spatial location). Add TbV log clustering by scene (i.e. spatial location). Mar 26, 2022
"""


ATX_CLUSTERS = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ATX_CLUSTERS = {
ATX_CLUSTERS: Final[Dict[int, Tuple[str, ...]]] = {

My suggestion is to make these map from int to tuples of strs since they're static data.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call @benjaminrwilson. Just updated to make this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like mypy interprets 1: ("mystring") as int: str instead of int: Tuple[str,...].

src/av2/datasets/tbv/scene_clusters.py:20: error: Dict entry 2 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"
src/av2/datasets/tbv/scene_clusters.py:21: error: Dict entry 3 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"
src/av2/datasets/tbv/scene_clusters.py:26: error: Dict entry 5 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"

I think I'll have to revert it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need to add a comma for a tuple literal?

E.g., 1: ("mystring",)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah. I think that's a bit uglier though. I would prefer we keep it as list.

@benjaminrwilson benjaminrwilson merged commit 15d43e3 into main Apr 12, 2022
@benjaminrwilson benjaminrwilson deleted the add-tbv-log-clustering-by-scene branch April 12, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants